Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rust-analyzer.toml tracking #16254

Closed
wants to merge 55 commits into from
Closed

rust-analyzer.toml tracking #16254

wants to merge 55 commits into from

Conversation

alibektas
Copy link
Member

@alibektas alibektas commented Jan 4, 2024 •
edited
Loading

Here you can see how far we have gotten with our planned .rust-analyer.toml ( aka RATOML configuration file ). The related issue is #13529. Our roadmap is roughly : ( this is a modified version of this comment.

Phase 1 :

  • Find out which configs are globally or locally or client-scoped. Create a PR where we possibly publish a document in which we briefly mention why a certain key-value pair is global or

So to make declaring a config as local/global/client simple I separated them into different configs. I am sure that I misplaced one or two configs here and there so we can always amend them.

  • Replace rust_analyzer::config::ConfigData with GlobalConfigData LocalConfigData. As a result of this change rust_analyzer::config::Config's data will be of type GlobalConfigData.

  • Integrating a deserializer : For this issue, we will use the simplest possible. When Taplo Integration Tracking #15908 becomes available we can migrate to it and start showing errors and auto-completion to the user. Many thanks to @cormacrelf for taking my ugly work and making wonders out of it!

  • project-model and all that lsp stuff : How do we get notified when a toml file get changed? How do we map a file to its corresponding RATOML file?

Some two months ago : (REJECTED) project_model::ProjectManifest is a good place to handle the discovery of the .rust-analyzer.toml file and finding out which configurations apply to a single ProjectManifest. This change will probably entail adding two more variants to ProjectManifest like ProjectManifest::ProjectJsonWithConf and ProjectManifest::CargoTomlWithConf. As I said before names are at this point arbitrary.

04.01.24 : This approach would make things more complicated than they already are. We will instead try making some changes to base-db::input::SourceRoot.

05.01.24 : Making changes to SourceRoot ( first discussed under #15888) so that we can express things like Fetch the related RATOML for this file has the same design flaw as 16170. So we need to store this information elsewhere. And as a side note, before even considering this option I thought about using a new database where I would map FileId ( of a source file ) to a FileId ( of a RATOML file ). This obviously would cause much redundancy as there needs to be one entry for each source file. Maybe we could consider using storing the information we want using a mapping of SourceRootId -> FileId sort.

  • Client settings and RATOML : During first handshakes between the server and the client config data are sent from the client to the server, which is how we have received configuration data so far. I plan to keep this as is and let server do a fresh start if any RATOML files are discovered. But this shouldn't tell you that the configs sent from the client have a higher priority than RATOML. Although I agree with Veykril on the point that our hand is forced to choose either RATOML or client settings over another, the user should still be able to say that they want to use their own configs, which means that we need to add a KV pair that does just that : telling us which source we should prefer.

  • First version of the RATOML : Although I may have sounded like this is something I want to have ready in the first version, I think we should first enable the use of a single RATOML file and gradually introduce nesting. So this version will use two sources to read configs from : (1) lsp_types::InitializeParams as sent by the client (2) RATOML.

Phase 2 :

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2024
@alibektas alibektas changed the title .rust-analyzer.toml tracking PR .rust-analyzer.toml tracking Jan 4, 2024
@alibektas
Copy link
Member Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2024
@alibektas alibektas changed the title .rust-analyzer.toml tracking rust-analyzer.toml tracking Jan 6, 2024
@alibektas
Make ConfigData Ser and TOML De
4196ad5 
In order to achieve this, I had to virtually rewrite ConfigData's default values to their typed versions. It ended up looking not so well.
I will think of something to fix this. In addition to this, some default value have been hastily modified but config manual generation will
be a stay as a reminder to fix this.
@cormacrelf
Copy link
Contributor

I kept going on config.rs. alibektas#1 targets this PR branch.

@cormacrelf cormacrelf mentioned this pull request Jan 18, 2024
Merged
@cormacrelf cormacrelf mentioned this pull request Jan 23, 2024
Merged
@cormacrelf
Copy link
Contributor

@davidbarsky : I'd be happy to send a PR adding support for something like figment or config.

See alibektas#2 for inheritable config, and alibektas#3 for a silly implementation of actually inheriting in a tree shape. What libraries like figment/config won't do is update when files change on disk. I also believe the plan is for the configuration to auto-update simply because the files are in the Vfs and get watched, so we do need a way to incrementally update the config. (My silly implementation should have just used salsa for that, but did not.) There is also a lot of custom-ness (aliases, tables to underscores, schema generation, ...) baked onto the json and by extension toml deserializers.

Similarly, I don't think that project_root/.rust-analyzer.toml should have precedence over editor-specified configuration

Agree, because the one in XDG_CONFIG_HOME will do the job of setting defaults, and does it way better. It will apply to all editors, for example. That leaves a natural job for client configs to override per-editor. So once this stuff lands most people can move what's in their init.lua/vscode config to XDG at their leisure, and eventually only use the client config for client-specific things like "don't insert close parens". No rush, no breaking changes.

The former consists of calling std::path::Path::ancestors and joining each result with rust-analyzer.toml

That's what I had in mind.

@davidbarsky
Copy link
Contributor

@davidbarsky : I'd be happy to send a PR adding support for something like figment or config.

See alibektas#2 for inheritable config, and alibektas#3 for a silly implementation of actually inheriting in a tree shape. What libraries like figment/config won't do is update when files change on disk. I also believe the plan is for the configuration to auto-update simply because the files are in the Vfs and get watched, so we do need a way to incrementally update the config. (My silly implementation should have just used salsa for that, but did not.) There is also a lot of custom-ness (aliases, tables to underscores, schema generation, ...) baked onto the json and by extension toml deserializers.

Oh duh, you're absolutely right. I didn't consider that those files would change over time!

Similarly, I don't think that project_root/.rust-analyzer.toml should have precedence over editor-specified configuration

Agree, because the one in XDG_CONFIG_HOME will do the job of setting defaults, and does it way better. It will apply to all editors, for example. That leaves a natural job for client configs to override per-editor. So once this stuff lands most people can move what's in their init.lua/vscode config to XDG at their leisure, and eventually only use the client config for client-specific things like "don't insert close parens". No rush, no breaking changes.

The former consists of calling std::path::Path::ancestors and joining each result with rust-analyzer.toml

That's what I had in mind.

Glad to hear it!

@alibektas
Copy link
Member Author

👋 Sorry for being inactive for the last couple of days.

There are certain issues that I want to draw your attention to

  • Problem with absolute paths : As the dev docs states paths are just implementation details. We need to be developing our solution on the interned Ids. So once we have interned a file, we shouldn't really be converting it back to do path manipulation to find the nearest ratoml file. The reason I am mentioning is that if I got the intention right, this should invalidate the approach mapping files to their corresponding ratomls ( given that we don't want to map a source file to ratoml by keeping a mapping , once again remember @davidbarsky's 1tb monorepo scenario ).

  • Salsa's role : Well, at this point it is premature. On the one hand we know that we will need incremental parsing at some point to enable autocompletion for toml files ( ratoml/Cargo.toml even regular toml files, afaik we can't really exclude non-ratoml/cargo.toml files from not being watched, let's hope that I am wrong ) but it should never be the case that the config-tree ,as @cormacrelf calls it, get updated every time salsa db gets and update. So on the materialization level configurations are bounded by the Vfs changes and on the autocompletion level by a SalsaDB that as frequently gets updated as the RootDatabase if that makes sense.

  • Changing some configs means it's time to restart the server but we didn't really do anything to handle this on the config level. In VSCode one would get notified when it's time to restart the server but this message isn't really coming from the rust-analyzer::config. We can improve on this.

@davidbarsky
Copy link
Contributor

👋 Sorry for being inactive for the last couple of days.

No worries, sorry for the delay in repsonding!

Problem with absolute paths : As the dev docs states paths are just implementation details. We need to be developing our solution on the interned Ids. So once we have interned a file, we shouldn't really be converting it back to do path manipulation to find the nearest ratoml file. The reason I am mentioning is that if I got the intention right, this should invalidate the approach mapping files to their corresponding ratomls ( given that we don't want to map a source file to ratoml by keeping a mapping , once again remember @davidbarsky's 1tb monorepo scenario).

I'm not sure that conversions after the fact are strictly necessary—I haven't thought about this enough, sorry!—but it seems like rust-analyzer.toml's will need to make use of the paths to determine their respective ordering, correct?

@alibektas
Copy link
Member Author

👋 Sorry for being inactive for the last couple of days.

No worries, sorry for the delay in repsonding!

Problem with absolute paths : As the dev docs states paths are just implementation details. We need to be developing our solution on the interned Ids. So once we have interned a file, we shouldn't really be converting it back to do path manipulation to find the nearest ratoml file. The reason I am mentioning is that if I got the intention right, this should invalidate the approach mapping files to their corresponding ratomls ( given that we don't want to map a source file to ratoml by keeping a mapping , once again remember @davidbarsky's 1tb monorepo scenario).

I'm not sure that conversions after the fact are strictly necessary—I haven't thought about this enough, sorry!—but it seems like rust-analyzer.toml's will need to make use of the paths to determine their respective ordering, correct?

Oh I forgot to write my follow up comment. Current strategy is the following and this makes my previous comment kinda obsolete.

  1. We are notified by Vfs that there is a new ratoml file.
  2. We get its ancestors all the way up to the corresponding workspace root.
  3. For all the ratomls that don't exist, the current implementation ask for FileIds as if they had been registered in Vfs. But this I think we will not have in the final version. Instead we will have them stored in a separate db and assign them a PlaceholderId until they actually exist.
  4. This way the finding out the closest ratoml for a file id is to ask vfs for its path take the path itself and gets its dirname and looking whether it currently exists or not, depending on which we get either a FileId or PlaceholderId and so on.

I think in 2-3 weeks we will have a prototype ready. The most complex example I have been thinking of testing my approach with is a scenario with linked projects and I gotta be honest. I have never dealt with rust-project.json. So I would appreciate if you could let me know if there is a certain thing unique to it that you want to see in ratoml, so that I can have it ready for the prototype.

@davidbarsky
Copy link
Contributor

For all the ratomls that don't exist, the current implementation ask for FileIds as if they had been registered in Vfs. But this I think we will not have in the final version. Instead we will have them stored in a separate db and assign them a PlaceholderId until they actually exist.

To confirm my understanding: that separate database will have PlaceholderIds corresponding to non-existent files (e.g., .rust-analyzer.toml, /a/foo.rust-analyzer.toml, /a/b/foo.rust-analyzer.toml) until a file is created and they are removed from that database?

The most complex example I have been thinking of testing my approach with is a scenario with linked projects and I gotta be honest. I have never dealt with rust-project.json. So I would appreciate if you could let me know if there is a certain thing unique to it that you want to see in ratoml, so that I can have it ready for the prototype.

Nothing at this time, but I'll give it a spin when it's ready!

@alibektas alibektas closed this Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cormacrelf cormacrelf cormacrelf left review comments

@davidbarsky davidbarsky davidbarsky left review comments

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alibektas @cormacrelf @davidbarsky @rustbot